crdutil: Add feature to also apply single files #58

tobiasgiese · 2024-11-25T09:23:48Z

This PR adds a feature to apply single yaml files, not only complete directories.

pkg/crdutil/crdutil.go

coveralls · 2024-11-25T09:27:23Z

Pull Request Test Coverage Report for Build 12053812919

Details

10 of 45 (22.22%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.08%) to 62.059%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/crdutil/crdutil.go	10	45	22.22%

Files with Coverage Reduction	New Missed Lines	%
pkg/crdutil/crdutil.go	2	37.72%

Totals
Change from base Build 11928826672:	-0.08%
Covered Lines:	1091
Relevant Lines:	1758

💛 - Coveralls

pkg/crdutil/crdutil.go

tobiasgiese · 2024-11-27T00:11:28Z

Tanks for the extensive review @shivamerla. I should have implemented all findings in this diff.

❯ go run cmd/apply-crds/main.go -f /tmp/foo/
2024/11/27 01:25:19 Apply CRDs from file: /tmp/foo/1.yaml
2024/11/27 01:25:19 Update CRD issuers.cert-manager.io

❯ go run cmd/apply-crds/main.go -f /tmp/foo/ -R
2024/11/27 01:25:22 Apply CRDs from file: /tmp/foo/1.yaml
2024/11/27 01:25:22 Update CRD issuers.cert-manager.io
2024/11/27 01:25:22 Apply CRDs from file: /tmp/foo/recursive/2.yaml
2024/11/27 01:25:22 Update CRD orders.acme.cert-manager.io

❯ go run cmd/apply-crds/main.go -f /tmp/foo/1.yaml
2024/11/27 01:25:28 Apply CRDs from file: /tmp/foo/1.yaml
2024/11/27 01:25:28 Update CRD issuers.cert-manager.io

❯ go run cmd/apply-crds/main.go -f /tmp/foo/1.yaml -f /tmp/foo/recursive/2.yaml
2024/11/27 01:25:35 Apply CRDs from file: /tmp/foo/1.yaml
2024/11/27 01:25:35 Update CRD issuers.cert-manager.io
2024/11/27 01:25:35 Apply CRDs from file: /tmp/foo/recursive/2.yaml
2024/11/27 01:25:35 Update CRD orders.acme.cert-manager.io

pkg/crdutil/crdutil.go

vasrem

LGTM overall, just one comment on the linter stuff.

pkg/crdutil/crdutil.go

vasrem · 2024-11-27T08:19:49Z

pkg/crdutil/crdutil.go

+				return nil
+			}
+			// If not recursive we want to only apply the CRDs in the top-level directory.
+			if !recursive && parentDir.IsDir() && filepath.Dir(path) != strings.TrimRight(crdDir, "/") {


should we add a test case for this new branch or should these all be included in the "TODO add unit tests" comment you have added?

Let me take another look at the tests, I think I can easily test the walkCRDs func

pkg/crdutil/crdutil.go

elezar · 2024-11-27T14:25:54Z

pkg/crdutil/crdutil.go

@@ -49,26 +51,32 @@ func (s *StringList) Set(value string) error {
 }

 var (
-	crdsDir StringList
+	files     []string
+	recursive bool


Does it make sense to separate the handling of multiple non-recursive paths (including files) from the addition of recursive handling?

AFAIK kubectl is also not separating it, so it is totally fine to keep it pretty open IMO. But also fine to me to split it somehow (you mean like: if multiple files are passed the recursive flag is not possible?)

I actually just meant as separate PRs. From NVIDIA/gpu-operator#1137 it seems that we don't use recursive mode. Is it used in the network operator?

No, it is not used by the network-operator. Maybe it makes sense to remove this new feature from the PR, you are right. The request adding it to this PR came through #58 (comment) (at least that's how I understood it)

elezar · 2024-11-27T14:28:13Z

pkg/crdutil/crdutil.go

-		log.Fatalf("CRDs directory is required")
+	pflag.CommandLine.AddGoFlagSet(flag.CommandLine)
+	pflag.StringSliceVarP(&files, "filename", "f", files,
+		"The files that contain the configurations to apply.")


This usage string does not seem to accurately represent the intent of the argument(s). From the PR description, I assume that each filename represents a path. Each of these can be a directory or a YAML file path. For directories, the YAML files in the directory are processed.

The flag names, the logic and also the description is copied from kubectl apply :)
This does not mean that it's perfect, it's just already kind of Kubernetes native

But to explain this a bit more:

the old behavior was indeed recursive by default, but without the possibility to decide this

the new behavior only handles yaml files in the same directory, if a directory is passed

with setting the recursive flag all directories will be handled recursively

Let me add tests for this to make this pattern clear

elezar · 2024-11-27T14:28:57Z

pkg/crdutil/crdutil.go

+	pflag.StringSliceVarP(&files, "filename", "f", files,
+		"The files that contain the configurations to apply.")
+	pflag.BoolVarP(&recursive, "recursive", "R", false,
+		"Process the directory used in -f, --filename recursively. Useful when you want to manage related manifests organized within the same directory.")


Is this not already handled without the -R flag? (This was the previous behaviour).

Same as #58 (comment)

elezar · 2024-11-27T14:29:31Z

pkg/crdutil/crdutil.go

 )

 func initFlags() {
-	flag.Var(&crdsDir, "crds-dir", "Path to the directory containing the CRD manifests")


Are we concerned about this as a breaking change since the command line arguments are changed?

I was also thinking about this but this pkg is pretty new and we are only using it for the network-operator currently. Do you think we should deprecate it?

If the network-operator is not released with this change, better to just update as it will be first use of this package.

The network-operator 24.10.0 has been released 2 days ago with the change in it, see https://github.com/Mellanox/network-operator/blob/v24.10.0/deployment/network-operator/templates/upgrade-crd-hook.yaml#L84.

But IMO it is totally fine to change it, because it is a Helm pre-install hook. If it fails, it fails the installation/upgrade and the user has to use the latest Helm chart. There is no dependency to the runtime which can cause the operator to fail.

WDYT @e0ne @rollandf

tl;dr: this PR changes the flag --crds-dir to --filename/-f

If there is a concern, we could support both -- marking crds-dir as deprecated -- for a release. Happy with the change since this is effectively an internal API.

We can also revert it to --crds-dir. Both crds-dir and filename does not contain both files and dirs, so it is not 100% explicit anyway :)
Request came through #58 (comment)

@shivamerla WDYT? Should we remove the flag rename and recursive flag (see #58 (comment)) out of this PR and change it in a follow-up? I'd say this will reduce the size of this PR.

pkg/crdutil/crdutil.go

elezar · 2024-11-27T14:32:33Z

pkg/crdutil/crdutil.go

-func walkCrdsDir(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface) error {
-	for _, crdDir := range crdsDir {
+// walkCRDs walks the CRDs directory and applies each YAML file.
+func walkCRDs(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface) error {


As a question: Does it makes sense to separate filename collection from applying the CRDs? That is to say that we would have logic that accepts the flags and returns a slice of YAML file paths. The logic to apply the CRDs would then accept a slice to YAML file paths. This should make things simpler to test and also allow us to properly de-duplicate if a user specifies a file multiple times, for example.

Yes, I think that makes totally sense. Let me change the code accordingly

Signed-off-by: Tobias Giese <[email protected]>

shivamerla

LGTM! Thanks

elezar · 2024-11-29T10:38:18Z

pkg/crdutil/crdutil.go

 		if _, err := os.Stat(crdDir); os.IsNotExist(err) {
-			log.Fatalf("CRDs directory %s does not exist", crdsDir)
+			log.Fatalf("CRDs directory %s does not exist", filenames)


should this be:

Suggested change

log.Fatalf("CRDs directory %s does not exist", filenames)

log.Fatalf("path does not exist: %s", cdrDir)

elezar · 2024-11-29T10:38:33Z

pkg/crdutil/crdutil.go

 	}

-	for _, crdDir := range crdsDir {
+	for _, crdDir := range filenames {


Suggested change

for _, crdDir := range filenames {

for _, path := range filenames {

elezar · 2024-11-29T10:39:43Z

pkg/crdutil/crdutil.go

@@ -84,38 +78,59 @@ func EnsureCRDsCmd() {
 		log.Fatalf("Failed to create API extensions client: %v", err)
 	}

-	if err := walkCrdsDir(ctx, client.ApiextensionsV1().CustomResourceDefinitions()); err != nil {
-		log.Fatalf("Failed to apply CRDs: %v", err)
+	dirsToApply, err := walkCRDs(recursive, filenames)


Suggested change

dirsToApply, err := walkCRDs(recursive, filenames)

pathsToApply, err := collectYamlPaths(recursive, filenames)

elezar · 2024-11-29T10:41:45Z

pkg/crdutil/crdutil.go

+// walkCRDs walks the CRDs directory and applies each YAML file.
+// TODO: add unit test  for this function.
+func walkCRDs(recursive bool, crdDirs []string) ([]string, error) {


Suggested change

// walkCRDs walks the CRDs directory and applies each YAML file.

// TODO: add unit test for this function.

func walkCRDs(recursive bool, crdDirs []string) ([]string, error) {

// collectYAMLPaths processes a list of paths and returns all YAML files.

// If `recursive` is specified directories in the list are searched recursively for YAML files.

// TODO: add unit test for this function.

func collectYAMLPaths(recursive bool, paths []string) ([]string, error) {

Note that we also need to rename some local variables to better indicate what we are collecting.

elezar · 2024-11-29T10:45:58Z

pkg/crdutil/crdutil.go

-// applyCRDsFromFile reads a YAML file, splits it into documents, and applies each CRD to the cluster.
-func applyCRDsFromFile(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface, filePath string) error {
+// applyCRDs reads a YAML file, splits it into documents, and applies each CRD to the cluster.
+func applyCRDs(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface, filePath string) error {


This rename wasn't strictly required, correct?

No, it's not required. It was a review finding #58 (comment)

elezar · 2024-11-29T10:48:21Z

pkg/crdutil/crdutil.go

@@ -170,14 +185,19 @@ func applyCRD(
 		if err != nil {
 			return fmt.Errorf("create CRD %s: %w", crd.Name, err)


Note: we already add crd.Name at the caller. Could we just return:

Suggested change

return fmt.Errorf("create CRD %s: %w", crd.Name, err)

return fmt.Errorf("create CRD: %w", err)

(and similarly for Update below)?

tobiasgiese commented Nov 25, 2024

View reviewed changes